Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add URI in allowed_names for cert auth #4231

Closed
wants to merge 2 commits into from
Closed

Add URI in allowed_names for cert auth #4231

wants to merge 2 commits into from

Conversation

nicholasjackson
Copy link
Contributor

This pull request adds the capability for URIs to be part of the allowed_names validation for certificates in the cert auth plugin. This will enable a user to authenticate vault using a SPIFFE SVID which contains a URI SAN for example spiffe://example.com/host.

Currently, I do not feel the tests are in the correct location and have only been implemented to check functionality. At present the test are implemented in the TestBackend_extensions_singleCert and the test fixtures for this test have been regenerated to add an example URI in the certificate extension. I think this should be in a completely separate test for example TestBackend_URI_singleCert and a new leaf cert /key generated containing these.

If you could provide some feedback on this please I would greatly appreciate it, also regarding the test fixtures, currently I am generating the fixtures using openssl not Vault:

# Generate the CSR
$ openssl req -new -config rootcawext.cnf -key rootcawextkey.pem -out rootcawext.csr

# Generate the cert
$ openssl x509 -req -days 365 -in rootcawext.csr -extfile rootcawext.cnf -extensions req_v3 -CA rootcacert.pem -CAkey rootcakey.pem -CAcreateserial -out rootcawextcert.pem

Would also appreciate some advice on the best way to generate the new test fixtures required for this change.

NOTE: This change uses go 1.10 specific features in the crypto/x509 package for the URIs

Kind regards,

Nic

@jefferai
Copy link
Member

Hi Nic,

Overloading allowed names for DNS and email is sort of a legacy implementation issue and I think uris should be in a separate parameter. (Eventually I think we should probably split up allowed names into e.g. allowed_dns_hostnames and allowed_email_addresses too.) That removes any ambiguity as to how a name is parsed; for instance if a uri is submitted that has an @ right now we'd assume it's an email address.

As for tests please make specific tests for this rather than inserting new steps; see the ones at the bottom of the test file for guidance.

@nicholasjackson
Copy link
Contributor Author

Cool thanks, I will add this as a separate parameter and separate the test as requested, happy to do the other stuff too for the split.

@jefferai jefferai added this to the 0.10.1 milestone Apr 1, 2018
@jefferai
Copy link
Member

Hi @nicholasjackson -- looks good. I'm happy with this as-is. You mentioned you might take on the work to split the other parameters, are you still thinking of doing that?

@nicholasjackson
Copy link
Contributor Author

Hey @jefferai, awesome news thank you, Sorry I have been so busy I completely missed your message. Yeah 100% happy to help with doing the split for other parameters. I will ping you on Slack later this week just so I am 100% on the new contract.

@jefferai jefferai modified the milestones: 0.10.1, 0.10.2 Apr 19, 2018
@robison
Copy link
Contributor

robison commented May 16, 2018

Hi, is there any way that I might be able to help on this PR? Seems like there's some functionality here that might be useful, especially around signing certs for users (with email addresses as the CN).

@jefferai
Copy link
Member

@nicholasjackson Am I right in thinking that #4463 totally supercedes this?

@jefferai jefferai modified the milestones: 0.10.2, 0.10.3 May 22, 2018
@nicholasjackson
Copy link
Contributor Author

@jefferai yes, #4463 was based on this PR however it adds the breakout for the api attributes, it might be best to close this one

@jefferai
Copy link
Member

Closed by #4463

@jefferai jefferai closed this May 25, 2018
@jefferai jefferai modified the milestones: 0.10.3, 0.10.2 May 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jefferai jefferai jefferai approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.10.2
Development

Successfully merging this pull request may close these issues.
3 participants
@nicholasjackson @jefferai @robison